Add tooltips to icon-only buttons - #264

@alex-rawlings-yyc resolved 1 discussion.
Reviewable status: 0 of 25 files reviewed, all discussions resolved.

@imnasnainaec reviewed 12 files and all commit messages, and made 5 comments.
Reviewable status: 12 of 25 files reviewed, 5 unresolved discussions (waiting on alex-rawlings-yyc).

src/components/ArcOverlay.tsx line 317 at r2 (raw file):

                    // `dark:hover:bg-background` is needed alongside the light one: the ghost variant
                    // dims to `dark:hover:bg-muted/50`, and Tailwind's merge treats `hover:bg-*` and
                    // `dark:hover:bg-*` as separate groups, so overriding one leaves the other standing.

⛏️ I'm not sure but this might be overexplaining tw functionality.

src/components/TokenChip.tsx line 391 at r2 (raw file):

              // alongside the light one: the ghost variant dims to `dark:hover:bg-muted/50`, and
              // Tailwind's merge treats `hover:bg-*` and `dark:hover:bg-*` as separate groups, so
              // overriding one leaves the other standing.

⛏️ I'm not sure but this might be overexplaining tw functionality.

src/components/TokenLinkIcon.tsx line 289 at r2 (raw file):

  // the slot straddles a segment boundary explains that instead — the reason outranks the action,
  // since naming an action the click won't perform is worse than naming none. The other disabled
  // reasons (unlink/edit mode) stay silent, their cause already visible in the UI.

⛏️ much of this comment appears to be pr-diff-justifying and lacking persistent value.

src/components/TokenLinkIcon.tsx line 356 at r2 (raw file):

  return (
    <Tooltip>
      {/* A disabled button is not a hover target, so the trigger rides a wrapper span that is. */}

⛏️ putting a span between a tooltip and a disablable child is standard procedure and doesn't require a comment.

src/components/tooltip-delay.ts line 4 at r2 (raw file):

 * Hover delay, in milliseconds, before any tooltip in the extension opens. The interlinear view and
 * the tab toolbar sit in separate React trees and so need a `TooltipProvider` each; both read this
 * value, so a tooltip does not open on a different clock depending on which tree it belongs to.

⛏️ "does not open on a different clock..." could be reworded away from the double-negative to a positive about what it does.

@alex-rawlings-yyc made 5 comments.
Reviewable status: 12 of 25 files reviewed, 5 unresolved discussions (waiting on imnasnainaec).

src/components/ArcOverlay.tsx line 317 at r2 (raw file):

Agreed — how tw-merge groups variants is Tailwind's to teach, not this file's. Cut to the constraint that actually has to hold, which is what a reader needs to know before touching the classes.

src/components/TokenChip.tsx line 391 at r2 (raw file):

Same trim as the ArcOverlay one.

src/components/TokenLinkIcon.tsx line 289 at r2 (raw file):

Agreed — the "the reason outranks the action" half was arguing for the choice rather than describing the code, which our comment rules call out directly ("the audience is the next reader of the code — never the reviewer of the current diff"). Kept the part that answers a real question for the next reader, namely which disabled states are deliberately silent.

src/components/TokenLinkIcon.tsx line 356 at r2 (raw file):

Agreed, dropped. It appeared twice in this file — the unlink trigger above had the same comment — so both are gone.

src/components/tooltip-delay.ts line 4 at r2 (raw file):

Reworded to the positive. Naming the two trees was also documenting consumers, which goes stale silently as trees are added or moved, so that went too.

@imnasnainaec reviewed all commit messages, made 1 comment, and resolved 5 discussions.
Reviewable status: 8 of 25 files reviewed, 1 unresolved discussion (waiting on alex-rawlings-yyc).

src/utils/phrase-arc.ts line 10 at r3 (raw file):

 * it, so only this half rises into the gap above.
 */
const CONTROLS_HALF_HEIGHT_PX = 12;

⛏️ Unless this "HALF" is half of something that still exist, Devin suggests something functionally descriptive like CONTROLS_ARC_OVERHANG_PX or CONTROLS_ARC_RISE_PX.

@alex-rawlings-yyc made 1 comment.
Reviewable status: 8 of 25 files reviewed, 1 unresolved discussion (waiting on imnasnainaec).

src/utils/phrase-arc.ts line 10 at r3 (raw file):

Done — renamed to CONTROLS_ARC_OVERHANG_PX, and swept the same stale "half" framing out of the computeStripTopPadding and computeStripRowGap docs.

It wasn't just vague — 12 hasn't been half of anything since CONTROLS_HEIGHT_PX = 28 landed as its own constant (28/2 is 14).

Left the value at 12; changing it to CONTROLS_HEIGHT_PX / 2 is a row-spacing change, not a naming fix.